Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🧹 [cleanup] - refactoring to reduce cyclomatic complexity in machine controller, add missing autogenerated changes #174

Merged
merged 1 commit into from
Mar 8, 2024
Merged

🧹 [cleanup] - refactoring to reduce cyclomatic complexity in machine controller, add missing autogenerated changes #174

merged 1 commit into from
Mar 8, 2024

Conversation

AshleyDumaine
Copy link
Contributor

@AshleyDumaine AshleyDumaine commented Mar 7, 2024 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer: Based on #172 which was just focused on some lint changes

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@AshleyDumaine AshleyDumaine added cleanup anything cleanup related go Pull requests that update Go code labels Mar 7, 2024
@AshleyDumaine AshleyDumaine marked this pull request as draft March 7, 2024 21:13
@AshleyDumaine AshleyDumaine changed the title 🧹 [cleanup] - refactoring to reduce cyclomatic complexity in machine controller 🧹 [cleanup] - refactoring to reduce cyclomatic complexity in machine controller, add missing autogenerated changes Mar 7, 2024
@AshleyDumaine AshleyDumaine marked this pull request as ready for review March 7, 2024 21:32
@AshleyDumaine
Copy link
Contributor Author

Created #175 to update GHA for diff checking (out of scope for this PR)

cbzzz
cbzzz previously approved these changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@cbzzz cbzzz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only you 👈🐻 can prevent complexity! 🔥🔥

controller/linodemachine_controller.go Outdated
Comment on lines 373 to 379
if linodeInstance == nil {
err = errors.New("missing instance")

logger.Error(err, "Panic! Failed to create isntance")

return nil, err
}
Copy link
Contributor

@cbzzz cbzzz Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally sure that we can remove this check? I assume it double-checks this part:

cluster-api-provider-linode/controller/linodemachine_controller.go

Lines 246 to 251 in 9a5dfbd

listFilter := util.Filter{
ID: machineScope.LinodeMachine.Spec.InstanceID,
Label: machineScope.LinodeMachine.Name,
Tags: tags,
}
linodeInstances, err := machineScope.LinodeClient.ListInstances(ctx, linodego.NewListOptions(1, listFilter.String()))

But I suspect this issue might be related to a something we hit before with the Linode Firewall, where if you call the linodego.Client with a bad(?) util.Filter you can get a nil object back with no error.

Base automatically changed from import-ordering to main March 8, 2024 14:50
@AshleyDumaine AshleyDumaine dismissed cbzzz’s stale review March 8, 2024 14:50

The base branch was changed.

@AshleyDumaine AshleyDumaine force-pushed the cyclop-fix branch 2 times, most recently from c9e04b2 to f57fd37 Compare March 8, 2024 15:07
@AshleyDumaine AshleyDumaine requested a review from cbzzz March 8, 2024 15:09
@AshleyDumaine AshleyDumaine force-pushed the cyclop-fix branch 2 times, most recently from 64d5a23 to 470be5d Compare March 8, 2024 16:41
@AshleyDumaine AshleyDumaine requested a review from nesv March 8, 2024 16:45
@AshleyDumaine AshleyDumaine requested a review from nesv March 8, 2024 17:21
nesv
nesv approved these changes Mar 8, 2024
View reviewed changes
Copy link

@nesv nesv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@AshleyDumaine AshleyDumaine merged commit 7788e07 into main Mar 8, 2024
6 of 7 checks passed
@AshleyDumaine AshleyDumaine deleted the cyclop-fix branch March 8, 2024 18:29
amold1 pushed a commit that referenced this pull request May 17, 2024
@AshleyDumaine
refactoring to reduce cyclomatic complexity in machine controller (#174)
f9249c3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nesv nesv nesv approved these changes

@eljohnson92 eljohnson92 Awaiting requested review from eljohnson92

@cbzzz cbzzz Awaiting requested review from cbzzz

Assignees
No one assigned
Labels
cleanup anything cleanup related go Pull requests that update Go code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AshleyDumaine @nesv @cbzzz